Skip to content

Conversation

@FoniksFox
Copy link
Contributor

Refactor SPI to the new compile infrastructure.

It is a refactor of the base driver, it discards all the logic that was built on top of the driver and mixed with it in the previous implementation.

Uses the new DMA (will update the code a little when it's final version comes arround).

Allows any desired spi config you may need.

If you want a protocol on top of it, you have to implement it (it is out of the scope for this pr, I'm planning on doing a general protocol class that's a mixin and can use any driver, format and operations). But it is pretty easy to use with packets (will add a better explanation in the wiki later):

  • Define all the packets you want to transmit via spi
  • Take the biggest packet size as the size of your buffers
  • Declare tx and rx buffers
  • Write and read packets to these buffers, use the id to know how to interpret the data, since you will allways be sending / receiving the max amount of data
  • To handle the case where the slave wants to send to the master, you either have a separate pin for it, or you make the master poll all the time and make the slave know it is a poll because it uses a specific id or something, I don't know, that's on you to decide

It uses a different api for master and slave to make clear the difference in behaviour (master instantly start the operation, while slave prepares it but must wait for the master)

Cantonplas and others added 30 commits November 17, 2025 20:01
…nically consteval), and a few more constexprs
…pp, fixed irqn function, and minor changes to inscribe_dma for legibility
…a cual se puede repetir). cambios en sus constructores y nuevo metodo (setDMAHandle) para poder pasar el handle desde el periferico
Copilot AI review requested due to automatic review settings January 27, 2026 23:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the SPI driver implementation to integrate with a new compile-time infrastructure. The refactor removes the protocol layer that was mixed with the base driver and introduces a cleaner separation of concerns using the new DMA infrastructure.

Changes:

  • Introduces new SPI2.hpp/cpp with compile-time configuration and separate master/slave APIs
  • Adds new DMA2.hpp/cpp with compile-time domain-based architecture
  • Comments out old SPI/DMA callback implementations in legacy code (FMAC, I2C, ADC, old SPI)
  • Updates ST-LIB.hpp to integrate new SPIDomain and DMA_Domain
  • Enables USE_FULL_ASSERT and changes library type from STATIC to OBJECT in build configuration

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
Inc/HALAL/Models/SPI/SPI2.hpp New SPI domain with compile-time configuration, pin validation, and separate master/slave wrapper APIs
Src/HALAL/Models/SPI/SPI2.cpp Implementation of IRQ handlers, HAL callbacks, and prescaler calculation for new SPI
Inc/HALAL/Models/SPI/SPIConfig.hpp New comprehensive SPI configuration types with translation to HAL constants
Inc/HALAL/Models/DMA/DMA2.hpp New DMA domain with compile-time configuration and peripheral-specific initialization
Src/HALAL/Models/DMA/DMA2.cpp DMA IRQ handlers for all 16 streams
Inc/ST-LIB.hpp Integration of new SPIDomain and DMA_Domain into build context
Src/HALAL/Services/Communication/SPI/SPI.cpp Old SPI callbacks commented out for future removal
Src/HALAL/Services/FMAC/FMAC.cpp DMA inscribe call commented out pending migration
Src/HALAL/Services/Communication/I2C/I2C.cpp DMA inscribe call commented out pending migration
Src/HALAL/Services/ADC/ADC.cpp DMA-related code removed, error message updated
Inc/HALAL/Services/FMAC/FMAC.hpp DMA stream fields removed from FMACInstance struct
Inc/HALAL/Services/Communication/SPI/SPI.hpp DMA stream fields removed from SPI Instance
Inc/HALAL/Services/Communication/I2C/I2C.hpp DMA stream fields removed from I2C Instance
Inc/HALAL/Services/ADC/ADC.hpp DMA stream removed from ADC InitData
Inc/HALAL/Models/Pin.hpp Updated PA15 alternate function mask
Inc/HALAL/HALAL.hpp Switched from old DMA.hpp to DMA2.hpp, added SPI2.hpp
Src/HALAL/HALAL.cpp Commented out old DMA::start() call
Src/HALAL/Models/DMA/DMA.cpp.tmp Added Spanish comment about coexistence with new DMA
Inc/HALAL/Models/DMA/DMA.hpp Minor formatting fix
Inc/stm32h7xx_hal_conf_template.h Enabled USE_FULL_ASSERT for debugging
CMakeLists.txt Changed library type from STATIC to OBJECT
Comments suppressed due to low confidence (1)

Src/HALAL/Models/DMA/DMA.cpp.tmp:10

  • This comment is written in Spanish and contains informal language. Comments in the codebase should be in English for consistency and international collaboration. The comment reads: "So that this code can coexist with the new dma, it has to be forced a little. I (Jorge not pm), believe that the new DMA can work for all peripherals. In case it is not like that, I will change this so it works (it will only be a moment)". This temporary comment should either be removed or properly translated and formalized.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +351 to +361
!compare_pin(sck_pin, PA7)) {
compile_error("Invalid SCK pin for SPI6");
}
if (!compare_pin(miso_pin, PB4) &&
!compare_pin(miso_pin, PG12) &&
!compare_pin(miso_pin, PA6)) {
compile_error("Invalid MISO pin for SPI6");
}
if (!compare_pin(mosi_pin, PB5) &&
!compare_pin(mosi_pin, PG14) &&
!compare_pin(mosi_pin, PA7)) {
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same pin PA7 is listed as valid for both SCK and MOSI on SPI6. This is likely an error as a single pin cannot serve two different SPI functions simultaneously. According to the STM32H7 datasheet, PA7 is typically an SPI1 MOSI pin, not an SPI6 pin. Please verify the correct pin mappings for SPI6 and update the validation accordingly.

Copilot uses AI. Check for mistakes.
* HAL drivers code
*/
/* #define USE_FULL_ASSERT 1U */
#define USE_FULL_ASSERT 1
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USE_FULL_ASSERT has been enabled (changed from commented out to defined as 1). This enables parameter assertion checks in the HAL drivers, which adds runtime overhead and code size. While this is useful for debugging, it should typically be disabled in release builds for optimal performance. Consider using conditional compilation based on build type (e.g., only enable in Debug builds) rather than enabling it unconditionally.

Copilot uses AI. Check for mistakes.
SPI_HandleTypeDef hspi;
SPI_TypeDef* instance;

volatile bool* operation_flag;
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operation_flag pointer in the Instance struct is not explicitly initialized. When Instance objects are created (as part of the instances array in Init), this pointer will have an indeterminate value. Although the callback checks for nullptr before dereferencing (line 101 in SPI2.cpp), relying on uninitialized memory to happen to be nullptr is undefined behavior. The pointer should be explicitly initialized to nullptr when the Instance is constructed to ensure predictable behavior.

Suggested change
volatile bool* operation_flag;
volatile bool* operation_flag = nullptr;

Copilot uses AI. Check for mistakes.
prescaler *= 2; // Prescaler doubles each step (it must be a power of 2)

if (prescaler > 256) {
ErrorHandler("Cannot achieve desired baudrate, speed is too low");
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message "Cannot achieve desired baudrate, speed is too low" is misleading. The condition that triggers this error is when the prescaler exceeds 256, which actually means the requested baudrate is too LOW (i.e., the source frequency cannot be divided down enough to achieve such a slow speed). The message should clarify that either the requested baudrate is too low for the given source frequency, or provide specific values to help with debugging (e.g., "Cannot achieve baudrate %u Hz with source frequency %u Hz").

Suggested change
ErrorHandler("Cannot achieve desired baudrate, speed is too low");
ErrorHandler("Cannot achieve requested baudrate: requested baudrate is too low for source frequency");

Copilot uses AI. Check for mistakes.
uint32_t buffer_length = peripheral->init_data.channels.size();
if (HAL_ADC_Start_DMA(peripheral->handle, (uint32_t*) peripheral->dma_data_buffer, buffer_length) != HAL_OK) {
ErrorHandler("DMA - %d - of ADC - %d - did not start correctly", peripheral->init_data.dma_stream, id);
ErrorHandler("DMA - %d - of ADC - %d - did not start correctly", id);
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message format is incomplete. It expects a format specifier for a DMA stream parameter (the first %d), but only one argument 'id' is provided. This will cause undefined behavior when the error is triggered. The format string should either be updated to match the number of arguments, or the missing DMA stream identifier should be passed as a second argument.

Suggested change
ErrorHandler("DMA - %d - of ADC - %d - did not start correctly", id);
ErrorHandler("DMA of ADC instance %d did not start correctly", id);

Copilot uses AI. Check for mistakes.
Comment on lines +548 to 559
// // void HAL_SPI_TxCpltCallback(SPI_HandleTypeDef* hspi) {}

// // void HAL_SPI_RxCpltCallback(SPI_HandleTypeDef* hspi) {}

// // void HAL_SPI_ErrorCallback(SPI_HandleTypeDef* hspi) {
// // if ((hspi->ErrorCode & HAL_SPI_ERROR_UDR) != 0) {
// // SPI::spi_recover(SPI::registered_spi_by_handler[hspi], hspi);
// // } else {
// // ErrorHandler("SPI error number %u", hspi->ErrorCode);
// // }
// // }

Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines contain double-commented code (// //). This appears to be an inconsistency where some code has been commented out twice. Either remove these lines entirely or use single-line comments consistently. The extra layer of commenting makes the code harder to read and suggests unclear intention about whether this code should be preserved or deleted.

Suggested change
// // void HAL_SPI_TxCpltCallback(SPI_HandleTypeDef* hspi) {}
// // void HAL_SPI_RxCpltCallback(SPI_HandleTypeDef* hspi) {}
// // void HAL_SPI_ErrorCallback(SPI_HandleTypeDef* hspi) {
// // if ((hspi->ErrorCode & HAL_SPI_ERROR_UDR) != 0) {
// // SPI::spi_recover(SPI::registered_spi_by_handler[hspi], hspi);
// // } else {
// // ErrorHandler("SPI error number %u", hspi->ErrorCode);
// // }
// // }

Copilot uses AI. Check for mistakes.
@@ -218,7 +218,7 @@ list(FILTER STLIB_HIGH_CPP_NO_ETH EXCLUDE REGEX "${STLIB_HIGH_ETH_REGEX}")
# Librería STLIB_LIBRARY
# ============================
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library type was changed from STATIC to OBJECT without explanation. OBJECT libraries behave differently than STATIC libraries - they don't create an archive file and their object files are directly linked into targets that use them. This change can affect link-time optimization, symbol visibility, and how the library is consumed by other targets. Please document why this change was made and verify it doesn't break existing build configurations or downstream projects that depend on this library.

Suggested change
# ============================
# ============================
# NOTE / NOTA:
# This target is intentionally an OBJECT library instead of STATIC.
# Historically this library was built as STATIC, but it was changed to
# OBJECT so that its object files can be directly reused by multiple
# firmware / executable targets while still being selected via generator
# expressions (CMAKE_CROSSCOMPILING, USE_ETHERNET, etc.).
#
# Compatibility:
# - Downstream projects must link against the CMake target `${STLIB_LIBRARY}`
# (e.g. `target_link_libraries(<app> PRIVATE ${STLIB_LIBRARY})`) and must
# not rely on a `.a` or `.lib` archive file name.
# - Existing build configurations that referenced the CMake target itself
# continue to work: they still see the same sources and compile options,
# only the internal representation changed from a STATIC archive to an
# OBJECT library.
# - No public interface of the library target has been changed by this
# switch; only the way CMake materializes it at link time is different.
#
# If a downstream project requires a real archive file, it should create its
# own STATIC or SHARED target that links against `${STLIB_LIBRARY}` or wraps
# its object files with `$<TARGET_OBJECTS:${STLIB_LIBRARY}>`.

Copilot uses AI. Check for mistakes.
Comment on lines +505 to +600
auto error_code = HAL_SPI_Transmit(&spi_instance.hspi, (uint8_t*)data.data(), data.size_bytes() / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Sends a trivially copyable data type over SPI in blocking mode.
*/
template <typename T>
bool send(const T& data) requires std::is_trivially_copyable_v<T> {
if (sizeof(T) % frame_size != 0) {
ErrorHandler("SPI data type size (%d) not aligned to frame size (%d)", sizeof(T), frame_size);
return false;
}
auto error_code = HAL_SPI_Transmit(&spi_instance.hspi, reinterpret_cast<const uint8_t*>(&data), sizeof(T) / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Receives data over SPI in blocking mode.
*/
template <typename E, size_t S>
bool receive(span<E, S> data) {
if (data.size_bytes() % frame_size != 0) {
ErrorHandler("SPI data size (%d) not aligned to frame size (%d)", data.size_bytes(), frame_size);
return false;
}
auto error_code = HAL_SPI_Receive(&spi_instance.hspi, (uint8_t*)data.data(), data.size_bytes() / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Receives a trivially copyable data type over SPI in blocking mode.
*/
template <typename T>
bool receive(T& data) requires std::is_trivially_copyable_v<T> {
if (sizeof(T) % frame_size != 0) {
ErrorHandler("SPI data type size (%d) not aligned to frame size (%d)", sizeof(T), frame_size);
return false;
}
auto error_code = HAL_SPI_Receive(&spi_instance.hspi, reinterpret_cast<uint8_t*>(&data), sizeof(T) / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Sends and receives data over SPI in blocking mode.
*/
template <typename E1, size_t S1, typename E2, size_t S2>
bool transceive(span<E1, S1> tx_data, span<E2, S2> rx_data) {
size_t size = std::min(tx_data.size_bytes(), rx_data.size_bytes());
if (size % frame_size != 0) {
ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size);
return false;
}
auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, (uint8_t*)tx_data.data(), (uint8_t*)rx_data.data(), size / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Sends and receives a trivially copyable data type over SPI in blocking mode.
*/
template <typename E, size_t S, typename T>
bool transceive(span<E, S> tx_data, T& rx_data) requires std::is_trivially_copyable_v<T> {
size_t size = std::min(tx_data.size_bytes(), sizeof(T));
if (size % frame_size != 0) {
ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size);
return false;
}
auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, (uint8_t*)tx_data.data(), reinterpret_cast<uint8_t*>(&rx_data), size / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Sends and receives a trivially copyable data type over SPI in blocking mode.
*/
template <typename T, typename E, size_t S>
bool transceive(const T& tx_data, span<E, S> rx_data) requires std::is_trivially_copyable_v<T> {
size_t size = std::min(sizeof(T), rx_data.size_bytes());
if (size % frame_size != 0) {
ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size);
return false;
}
auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, reinterpret_cast<const uint8_t*>(&tx_data), (uint8_t*)rx_data.data(), size / frame_size, 10);
return check_error_code(error_code);
}

/**
* @brief Sends and receives a trivially copyable data type over SPI in blocking mode.
*/
template <typename T1, typename T2>
bool transceive(const T1& tx_data, T2& rx_data) requires (std::is_trivially_copyable_v<T1> && std::is_trivially_copyable_v<T2>) {
size_t size = std::min(sizeof(T1), sizeof(T2));
if (size % frame_size != 0) {
ErrorHandler("SPI transaction size (%d) not aligned to frame size (%d)", size, frame_size);
return false;
}
auto error_code = HAL_SPI_TransmitReceive(&spi_instance.hspi, reinterpret_cast<const uint8_t*>(&tx_data), reinterpret_cast<uint8_t*>(&rx_data), size / frame_size, 10);
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All blocking SPI operations use a hardcoded timeout value of 10 milliseconds. This magic number should be either: (1) defined as a named constant to make it clear what it represents and allow for easy modification, or (2) made configurable per-device to accommodate different use cases. Some SPI operations may require longer timeouts depending on the slave device and data size, while others may need shorter timeouts.

Copilot uses AI. Check for mistakes.
Comment on lines +266 to +363
if (!compare_pin(sck_pin, PB3) &&
!compare_pin(sck_pin, PG11) &&
!compare_pin(sck_pin, PA5)) {
compile_error("Invalid SCK pin for SPI1");
}
if (!compare_pin(miso_pin, PB4) &&
!compare_pin(miso_pin, PG9) &&
!compare_pin(miso_pin, PA6)) {
compile_error("Invalid MISO pin for SPI1");
}
if (!compare_pin(mosi_pin, PB5) &&
!compare_pin(mosi_pin, PD7) &&
!compare_pin(mosi_pin, PA7)) {
compile_error("Invalid MOSI pin for SPI1");
}
break;

case SPIPeripheral::spi2:
if (!compare_pin(sck_pin, PD3) &&
!compare_pin(sck_pin, PA12) &&
!compare_pin(sck_pin, PA9) &&
!compare_pin(sck_pin, PB13) &&
!compare_pin(sck_pin, PB10)) {
compile_error("Invalid SCK pin for SPI2");
}
if (!compare_pin(miso_pin, PC2) &&
!compare_pin(miso_pin, PB14)) {
compile_error("Invalid MISO pin for SPI2");
}
if (!compare_pin(mosi_pin, PC3) &&
!compare_pin(mosi_pin, PC1) &&
!compare_pin(mosi_pin, PB15)) {
compile_error("Invalid MOSI pin for SPI2");
}
break;

case SPIPeripheral::spi3:
if (!compare_pin(sck_pin, PB3) &&
!compare_pin(sck_pin, PC10)) {
compile_error("Invalid SCK pin for SPI3");
}
if (!compare_pin(miso_pin, PB4) &&
!compare_pin(miso_pin, PC11)) {
compile_error("Invalid MISO pin for SPI3");
}
if (!compare_pin(mosi_pin, PB5) &&
!compare_pin(mosi_pin, PD6) &&
!compare_pin(mosi_pin, PC12) &&
!compare_pin(mosi_pin, PB2)) {
compile_error("Invalid MOSI pin for SPI3");
}
break;

case SPIPeripheral::spi4:
if (!compare_pin(sck_pin, PE2) &&
!compare_pin(sck_pin, PE12)) {
compile_error("Invalid SCK pin for SPI4");
}
if (!compare_pin(miso_pin, PE5) &&
!compare_pin(miso_pin, PE13)) {
compile_error("Invalid MISO pin for SPI4");
}
if (!compare_pin(mosi_pin, PE6) &&
!compare_pin(mosi_pin, PE14)) {
compile_error("Invalid MOSI pin for SPI4");
}
break;

case SPIPeripheral::spi5:
if (!compare_pin(sck_pin, PF7)) {
compile_error("Invalid SCK pin for SPI5");
}
if (!compare_pin(miso_pin, PF8)) {
compile_error("Invalid MISO pin for SPI5");
}
if (!compare_pin(mosi_pin, PF9) &&
!compare_pin(mosi_pin, PF11)) {
compile_error("Invalid MOSI pin for SPI5");
}
break;

case SPIPeripheral::spi6:
if (!compare_pin(sck_pin, PB3) &&
!compare_pin(sck_pin, PG13) &&
!compare_pin(sck_pin, PC10) &&
!compare_pin(sck_pin, PA7)) {
compile_error("Invalid SCK pin for SPI6");
}
if (!compare_pin(miso_pin, PB4) &&
!compare_pin(miso_pin, PG12) &&
!compare_pin(miso_pin, PA6)) {
compile_error("Invalid MISO pin for SPI6");
}
if (!compare_pin(mosi_pin, PB5) &&
!compare_pin(mosi_pin, PG14) &&
!compare_pin(mosi_pin, PA7)) {
compile_error("Invalid MOSI pin for SPI6");
}
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pin PC10 is listed as valid for both SPI3 (line 304) and SPI6 (line 350) SCK pins. Additionally, PB3 is listed for SPI1, SPI3, and SPI6 SCK, and PB4 is listed for SPI1, SPI3, and SPI6 MISO. While it's possible for pins to support multiple peripherals through alternate functions, this should be verified against the STM32H7 datasheet. If these are indeed valid, users need to be careful not to configure multiple SPI peripherals with overlapping pins simultaneously.

Copilot uses AI. Check for mistakes.
prescaler *= 2; // Prescaler doubles each step (it must be a power of 2)

if (prescaler > 256) {
ErrorHandler("Cannot achieve desired baudrate, speed is too low");
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After calling ErrorHandler when prescaler exceeds 256, the function continues execution and attempts to call get_prescaler_flag with an invalid prescaler value (>256). The function should either return a default prescaler value or use a more severe error handling mechanism that doesn't return. Consider adding 'return SPI_BAUDRATEPRESCALER_256;' after the ErrorHandler call to provide a fallback behavior.

Suggested change
ErrorHandler("Cannot achieve desired baudrate, speed is too low");
ErrorHandler("Cannot achieve desired baudrate, speed is too low");
return SPI_BAUDRATEPRESCALER_256;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants